Skip to content

Conversation

@jpfeuffer
Copy link
Contributor

@jpfeuffer jpfeuffer commented Nov 18, 2023

Summary

This PR enables nogil support for most types of methods in autowrap-generated code. When a method is marked with nogil in the PXD file (e.g., void process() nogil), the generated wrapper will release Python's Global Interpreter Lock (GIL) before calling into C++, allowing true parallelism.

Key Changes

  • ConversionProvider API change: input_conversion() now returns a 4-tuple (code, call_as, cleanup, decl) where decl is a tuple (declaration, call_expression) for split declaration in nogil contexts
  • call_method() signature: Added with_cdef parameter to control whether to include cdef (not allowed inside nogil: blocks)
  • CodeGenerator: Updated to handle the new return values and generate proper nogil-compatible code
  • Cython requirement: Updated to >=3.1 for improved nogil support

Updated Converters

All TypeConverter classes now support nogil:

  • Primitive types (int, float, bool, char, etc.)
  • String types (libcpp_string, const_char*, string_view)
  • Container types (vector, map, set, list, deque, pair)
  • C++17 types (optional, unordered_map, unordered_set)
  • Wrapped class types
  • Enum types

nogil + OpenMP Thread Interaction Analysis

When a nogil-marked method (e.g., in pyOpenMS) internally uses OpenMP for parallelism:

How It Works

  1. GIL is released before entering the C++ code via with nogil: block
  2. OpenMP threads spawn inside the C++ library
  3. All OpenMP threads execute without competing for Python's GIL
  4. Threads complete, control returns
  5. GIL is reacquired when leaving the nogil block

Safety Considerations

Aspect Behavior
GIL during OpenMP Released - OpenMP threads run freely
Thread safety Safe as long as C++ code doesn't call back into Python
Python callbacks Would need with gil: to reacquire GIL first
Other Python threads Can run concurrently while OpenMP works

Example Flow

Python: call pyopenms.method()
  └─ Cython: "with nogil:" releases GIL
       └─ C++ code runs
            └─ #pragma omp parallel spawns threads
                 └─ All threads work without GIL contention
            └─ OpenMP threads complete
       └─ Returns to Cython
  └─ GIL reacquired
Python: continues

Bottom Line

This is safe and beneficial for libraries like OpenMS that use OpenMP internally:

  • The GIL doesn't block OpenMP threads
  • OpenMS's OpenMP code is pure C++ (no Python callbacks)
  • Other Python threads can run while the C++ library processes data
  • True multi-core parallelism is achieved

Test Plan

  • All existing tests pass (165 passed, 1 skipped due to environment)
  • C++17 STL containers test passes (optional, string_view, unordered_map, unordered_set)
  • GIL testing with gil_testing.hpp verifies GIL is properly released

References

This requires a change in conversion providers.
+Some other cleanups.
@jpfeuffer
Copy link
Contributor Author

jpfeuffer commented Nov 18, 2023

I will fix tests because of newer typing syntax

Copy link
Contributor

@poshul poshul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Any idea what sort of performance improvements we are likely to see?

@jpfeuffer
Copy link
Contributor Author

Probably none out of the box.
But it allows to use our functions in a multi threaded python context e.g. via the python threading module.

This brings some responsibilities for the user with it but you can decide which functions are safe to use.

Copy link
Collaborator

@timosachsenberg timosachsenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not super confident that I understand all parts. Would be great if in general methods would contain a brief comment e.g.the new _split one etc.

@jpfeuffer
Copy link
Contributor Author

This is a breaking change. Be prepared to fix OpenMS if you merge this.

@poshul
Copy link
Contributor

poshul commented Apr 16, 2024

Given that we already need to do some fixes for OpenMS to get it to work with the current develop version of autowrap, I'm good merging this in and planning to have the core group sit down and fix at some point in the future.

@timosachsenberg
Copy link
Collaborator

what needs to be done to work with the current develop version of autowrap otherwise?

@timosachsenberg
Copy link
Collaborator

and what would need to be fixed?

@poshul
Copy link
Contributor

poshul commented Apr 16, 2024

and what would need to be fixed?

#186

@jpfeuffer
Copy link
Contributor Author

@timosachsenberg @claude This might be something for you guys. Maybe we should even default to wrapping without GIL. I am only wondering what happens if you start a threadpool in python and then each thread calls a function parallelized with OpenMP.

@timosachsenberg
Copy link
Collaborator

ok I can take a look. can you review the other PRs? especially the memory view one could require some input what you had in mind

@timosachsenberg
Copy link
Collaborator

Additional commit available

A new commit has been pushed to OpenMS/autowrap:fix/nogil that completes the nogil support:

Commit: 2306ff7 - Complete nogil support for remaining converters

This fixes the C++17 STL containers (StdOptionalConverter, StdStringViewConverter) and other converters that were missing the 4th decl return value from input_conversion().

Changes in this commit:

  • StdVectorAsNumpyConverter
  • StdUnorderedMapConverter
  • StdUnorderedSetConverter
  • StdDequeConverter
  • StdListConverter
  • StdOptionalConverter
  • StdStringViewConverter

All now return the proper 4-tuple and have with_cdef parameter in call_method().

Tests: 165 passed, 1 skipped (environment issue unrelated to changes)

To merge this, either:

  1. Cherry-pick commit 2306ff7 from OpenMS/autowrap:fix/nogil into this PR's branch
  2. Or I can open a separate PR if preferred

@timosachsenberg
Copy link
Collaborator

based and finished (kind of) in other pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants